Skip to content

Add support for params.expect#855

Merged
Burgestrand merged 5 commits into
varvet:mainfrom
Linuus:expected-attributes
Jan 8, 2026
Merged

Add support for params.expect#855
Burgestrand merged 5 commits into
varvet:mainfrom
Linuus:expected-attributes

Conversation

@Linuus

@Linuus Linuus commented Mar 27, 2025

Copy link
Copy Markdown
Collaborator

Fixes: #841

To do

  • I have read the contributing guidelines.
  • I have added relevant tests.
  • I have adjusted relevant documentation.
  • I have made sure the individual commits are meaningful.
  • I have added relevant lines to the CHANGELOG.

This PR starts adding support for params.expect in Rails which is now recommended over params.require.permit.
See: https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect

I'm a bit unsure what to do with pundit_params_for though. It's kind of redundant... but also kind of useful as mentioned in our Readme, if you want to fetch based on another "namespace" than the default. However, for expect it would essentially just turn into a wrapper method for PolicyFinder.new(record).param_key, so you can just override the param_key method on the Policy instead? 🤔

Comment thread spec/authorization_spec.rb Outdated
@Linuus Linuus self-assigned this Mar 28, 2025
@Linuus Linuus requested a review from Burgestrand March 28, 2025 13:47
@Burgestrand

Copy link
Copy Markdown
Member

This looks as one could expect (:D). I'm also wondering if this is a good time to adjust what kind of API we will permit (:D), since this is an addition we've got time to adjust the API without breaking any backwards-compatibility.

Things I'm thinking of:

  • what if our policy API is expected_params_for(action), let the receiver worry about different params for different actions?
  • param key, is it worth moving it to pundit.param_key(record) so it delegates to the proper policy finder

We're still rather light on Pundit-usage in our current projects. What about your project, do you use the action-specific expected_params?

@jensljungblad-kisi

Copy link
Copy Markdown

What about your project, do you use the action-specific expected_params?

We do, yeah.

@Linuus

Linuus commented May 1, 2025

Copy link
Copy Markdown
Collaborator Author

What about your project, do you use the action-specific expected_params?

We do, yeah.

Well we are not using the expect at all yet as we are waiting for it to be added in Pundit, right? :)

@jensljungblad-kisi

Copy link
Copy Markdown

Correct, I thought the question was if we're using the existing action specific permitted attributes methods.

@Burgestrand

Copy link
Copy Markdown
Member

Ah yes. Sometimes I write a bit för fort!

I haven't forgotten this. Still thinking about if I want to adjust the future API now that there's a chance to do so.

@matthew-puku

matthew-puku commented Jun 5, 2025

Copy link
Copy Markdown

We use permitted_attributes 41 times and permitted_attributes_for_x 3 times.

  • what if our policy API is expected_params_for(action), let the receiver worry about different params for different actions?

For us that means that in most cases the methods would look like:

def expected_params_for(_action)
  %i[foo bar]
end

The action-specific cases would take one of two forms:

# 1 Switch
def expected_params_for(action)
  case action.to_sym
  when :create
    %i[foo bar]
  when :update
    [:bar, (:foo if user.admin?)].compact
  else
    %i[bar]
  end
end

# 2 Concat
def expected_params_for(action)
  params = %i[foo]
  params += %i[bar baz] if action.to_sym == :create
  params
end

Things I'd note:

  • It's not immediately clear to me whether action would be a symbol or a string.
  • We try to avoid custom actions, so for our project the action argument is essentially a bool.
  • The methods get gnarly if you have conditions on action and conditions on other stuff (as we usually do).

All that to say, I think I prefer copying the existing policy API ala expected_attributes_for_x, but it's not a huge deal either way.

@Burgestrand

Copy link
Copy Markdown
Member

@matthew-puku Thanks, this is great feedback!

@jensljungblad-kisi

Copy link
Copy Markdown

Any plans to proceed with this? Can we do anything to help?

@Burgestrand

Burgestrand commented Dec 22, 2025

Copy link
Copy Markdown
Member

Any plans to proceed with this? Can we do anything to help?

Yeah! I'm on a project now where I'll start using Pundit a bit more again, so that should allow me to try things out. Also christmas holidays 😄 I still have this irk that I want to finish up all issues, including the many-years old ones.

1. I do want to move the responsibility of public_send("expected_params_for_#{action}") into the receiver 1, e.g. we'll instead be calling expected_params_for_action(action). I don't mind that it'll often be ignored, I could see this implementation be okay in a specific project, if that's the norm:

def expected_params_for_action(_action) = expected_params

I've considered the issue about data type and it'll semantically be whatever the framework you're using is passing, i.e. Rails' action_name.

2. I also want to move the param_key responsibility into Pundit::Context2, mainly because it's a place where we can use dependency injection to introduce changes while avoiding breaking backwards-compatibility. If I ever do introduce the concept of "current namespace", then that will live inside the context too and might impact the policy finder, which we delegate to in order to find the param_key 3.

I think it's as simple as just adding the method:

def param_key(record) = policy_finder(record).param_key

Footnotes

  1. I think there's a logical issue here in the previous implementation, too. I'm not even sure we should ever "fall back" to a generic permitted parameters; it's safer to always have specific permitted parameters per action, or always have the same permitted parameters for all actions. If you forget a definition or have a typo (e.g. permitted_params_for_updat) we should complain, rather than fall back to a default (permitted_params). Just throwing it out there as an FYI, because we can't change it due to backwards-compatibility anyway and my opinion is that we should delegate this to the receiver anyway.

  2. I have but one apprehension on this entire concept of the context, and it's that it might be more difficult to extend. I believe a strong part about Pundit is that it's small and simple to the point where monkey-patching and re-implementing Pundit methods is a good way of approaching certain things. Moving implementation into a class might make that more rigid, so I'm being rather cautious.

  3. I believe the PolicyFinder might be doing a bit too much, responsibility-wise.

@Burgestrand Burgestrand force-pushed the expected-attributes branch 2 times, most recently from a289ca9 to 79101b5 Compare December 28, 2025 10:46
Linuus and others added 4 commits January 8, 2026 11:43
1. Public API surface is reduced compared to multiple methods.
2. We retain the flexibility of action-specific expected attributes.

If you still prefer the old style, you can implement it yourself in your policy:
```
def expected_params_for_action(action_name)
  if respond_to?("expected_params")
    expected_params
  else
    public_send("expected_params_for_#{action_name}")
  end
end
```
@Burgestrand

Burgestrand commented Jan 8, 2026

Copy link
Copy Markdown
Member

FYI I ended up rebasing this branch on main, rather than merging it into this one. Feature-wise I think we should be good now, and hopefully all checks pass now that I've updated the GH Actions matrix.

I'll let this marinate while I go to lunch, but I intend to merge it today.

I would be interested to hear your experiences if you're willing to try out running pundit from the main branch, to test expected attributes out.

@Burgestrand Burgestrand merged commit d53c841 into varvet:main Jan 8, 2026
14 checks passed
@Burgestrand

Burgestrand commented Jan 8, 2026

Copy link
Copy Markdown
Member

Thank you! While this has been merged, I'm more than happy to keep options for improving this open. Once it's finally released the API should remain stable, but until then it's a free for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support new params expect method to permit parameters

4 participants